-
Notifications
You must be signed in to change notification settings - Fork 143
Reapply "Big form widgets refactor to allow user-defined widgets, added number widgets (#991)" #1001
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…ed number widgets (web2py#991)" This reverts commit a1f6238.
|
I merged this again but then had to revert it again. My bad — sorry. There are a few things that still need to be addressed: There should not be a global registry of widgets exposed to users. If an app touches the global registry, it changes the behavior for all other apps. I think there should be a function that returns the default list, which the user can then edit (add/remove/replace widgets) and pass to the form. The function could be called get_default_widgets(). The get_widget_for function has a loop to find the best match. This makes it O(N), and I prefer O(1). I think the widget needs to be identified exclusively by field.name (if overridden for a specific field) or field.type_name, using at most two lookups without any pattern matching. So the registry returned by get_default_widgets() would be a dict, and its keys would be "type:boolean", "type:string", etc. Users could add their own "type:string" (to replace defaults for their own forms) or "field:first_name" to declare one for a specific field. There should be no "type:radio" or "type:select" because those are not valid field.type_name values. It’s unclear whether the field registry should be passed to the form via the FormStyle or as a separate object. In my ideal world, they would be two separate inputs to Form that the user can mix and match — but this may not be possible because a style may dictate the widget’s HTML structure. In that case, the registry belongs in the style. Otherwise, I like the rest of the code. If we agree, feel free to refactor — or I can attempt a refactor myself. |
Hmm, i understand your point. The registry was partially intended for libraries extending which widgets are available, which would be fine, but the fact its a single global one isn't great for multitple apps running in the same py4web process. (which is a practice i strongly disagree with, I think if you want multiple apps (beyond just the dashboard) you should keep them actually separate. Because i promise you this registry is not the only place state leaks across app boundaries). I still want to keep something like it, instead of continuing the annoying design of having to pass tons of things to each Form, which isnt DRY at all. But i think i can finagle it to be a app-local registry, probably using a ChainMap to resolve it or similar.
This was a very conscious decision on my part, because lookups by just field name or just field type are quite limiting. If you can show me that loop actually has a noticeable performance impact i'll relent, but i've profiled py4web apps and trust me, nobody will notice the search loop. Yes, its worse Big-O, but Big-O is the general performance characteristic for any size dataset. In this case, where there will be a few dozen at most, the absolute value is incredibly small. To be sure, i did a quick timeit benchmark of dict lookup vs linear search, both over randomized datasets: Script
import timeit
setup = """
import random
l = [(i, 'somedata') for i in range(50)]
random.shuffle(l)
"""
search = """
for (i, data) in l:
if i == 25: break
"""
d_setup = setup + "\nd = dict(l)"
d_lookup = "d[25]"
all_l = timeit.repeat(stmt=search, setup=setup, repeat=100000, number=50)
all_d = timeit.repeat(stmt=d_lookup, setup=d_setup, repeat=100000, number=50)
import statistics
print(f"list: {statistics.mean(all_l)*1000000}μs")
print(f"dict: {statistics.mean(all_d)*1000000}μs")Ultimately i've intended this widgets system to be more flexible than just reimplementing the list of if-elif checks for type, and i've already found great use for the more flexible matching in the py4web project i'm working on. I didn't implement it this way out of nowhere. |
|
You can treat this matching system as me caching in some of the performance budget i've freed up with my threadsafevariable optimizations 😉 |
|
How would you like to proceed @mdipierro ? I could re-do the PR again, but I don't want to put effort in a solution you don't agree with.
|
|
I feel somewhat strongly about the first point. I agree with you that one app can always access global variables and mess things up for other but, at the very least, we should not encourage it. It would be fine with just renaming widgets as widgets and a get_default_widgets() that returns copy.copy(widgets) which users can then pass to grid and forms. About the patter matching. I am not opposed to it, in particular given your benchmarks. Yet, I think it will confuse users. They will not always understand why one widget is chosen over another. Also minor tweaks to the code over time may result in breaking apps' behavior. I think users will have a much easier time if the widgets is chosen exclusively based on the field.type_name and if this default can be overwritten using field.fullname. Perhaps the key would be something like (None,"boolean") or ("tablename.fieldname",None) and Also a a widgets could delegate to another widget. So there would be a super widget that can handle multiple options and delegate to a widget that can handle a single option which then can delegate to a more elementary widget based exclusively on type. The superwidget would be the default for lists/strings/numbers which may or may not have options. I think it would be sufficiently powerful to handle most scenarios and easy enough to understand and override. Happy to discuss more pros and cons. If you want I can implement something like this based on your existing PR. But you seem faster than me. ;-) |
This reverts commit a1f6238 which itself originally reverted #991.
Fixes: